-
-
Notifications
You must be signed in to change notification settings - Fork 595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix bugzilla Issue 24397 ImportC: support function-like macros #16199
Conversation
Thanks for your pull request, @WalterBright! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#16199" |
e5e8430
to
cc8c006
Compare
It's very frustrating when all the test suite Ubuntus fail with a seg fault, yet everything works fine on my local Ubuntu box. |
b88f83e
to
e639cdb
Compare
I'm dead in the water on this. I cannot repro any failures on my Ubuntu machine. None of the test machines build the compiler with -debug, so the crash dumps are nonexistent or useless. If I add logging printfs, the resulting output is so large the ci machines hang. |
e639cdb
to
344a6e5
Compare
Have you tried podman/docker to run it in a 22.04 system environment? |
I can reproduce it locally. The failure happens in addVar, because defineTab[cast(void*)s.ident] = symbols.length; |
@WalterBright this seems to be the reduction that confirms my observation.
|
@tim-dlang @ibuclaw I sure appreciate your help with this! I will set about fixing this. |
4ce697a
to
8bd9815
Compare
8bd9815
to
ee031a0
Compare
@ibuclaw I can confirm that your reduction fails on my machine. Thanks! |
3e8e812
to
e6aa692
Compare
I'm mystified by this, as this PR is only about D code using #defines, it doesn't have anything to do with how the C preprocessor works. Maybe the FreeBSD preprocessor is different? Unfortunately, my FreeBSD machine is broken at the moment. |
The OMF DMD is also failing, and I can repro that, so I'll investigate it first. |
I have a VM, and I see that this PR causes the regression. Command:
Before:
After:
No C symbols are being emitted. I have an idea of what this might be. |
This is the reproducer: #define M16199C(X,S,M) ({ int __x; })
int pr16199c()
{
return 0;
} All functions after the macro definition get erroneously swallowed up, so the result ends up being: extern (C) auto M16199C(__MP1, __MP2, __MP3)(__MP1 X, __MP2 S, __MP3 M)
{
return delegate ()
{
{
extern (C) int pr16199c()
{
return 0;
}
extern (C) int __x = void;
;
}
}
();
} |
7f66ec3
to
9813e87
Compare
damn, you're good, @ibuclaw ! |
This is the behaviour seen on FreeBSD that causes nothing to be emitted. #define M16199Da(TYPE,VAR) ((TYPE)(VAR))
#define M16199D(X,S,M) ({ int *__x = (X); M16199Da(S *, __x); })
int pr16199d() { return 0; } Tweak it a bit more, and we get a segfault in cparse. #define M16199Ea(TYPE) (TYPE __x;)
#define M16199E(X,S,M) ({ M16199Ea(S *); }) |
Ouch! Anyhow, the OMF problem is fixed. |
3d0f5b2
to
421fd06
Compare
421fd06
to
5232470
Compare
@ibuclaw the problems you identified seem to be fixed now. There were 3 bugs I found and fixed. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some observations for future iterations
-
It would be good to support
do {} while(0)
function-like macros. -
I see that
#define N (42)
is converted intoauto N()() => 42
where a simple lookahead might be able to detect the lone literal in parens and simplify it asenum N = 42
-
Do these macros occupy storage space if instantiated? ie: By wrapping it in an eponymous template, it looks like it would be possible to take the address of a C macro from D. What about an enum function/delegate (think
enum ADD(__MP1, __MP2) = (__MP1 a, __MP2 b) { return a + b; }
) instead?
The translation of macro functions to D functions is not correct in general because of arguments with side-effects. Consider: #define DOUBLE(x) (x) + (x)
void f(void)
{
int i = 0;
DOUBLE(i++);
assert(i == 2); // fails after D translation
} Which is a silly example, but I've encountered real C macros that rely on this behavior. |
Maybe it would be enough to ignore macros that reuse an argument? |
@dkorpel of course you're right. There are other problems like if the macro body deliberately did not enclose a parameter in ( ), or even the entire expansion in ( ). More or less, if the macros are intended for use as metaprogramming rather than as template-like, there's going to be trouble. I don't think it is possible for ImportC to promise to replicate C preprocessor semantics. The best we can do is make our best shot. And document things that won't work, like your example. |
@ntrel it's a good idea, but it also then may annoyingly refuse to compile things that would otherwise work as intended. |
Not always. I've mistranslated macros to functions before, and it's annoying to debug. I'd rather have ImportC fail to translate a macro than produce an incorrect result. |
Doc PR: dlang/dlang.org#3771 |
I understand your concern, and it bothers me, too. I pointed out some other cases in the doc PR. A couple people have pointed out that not supporting function-like macros makes some C packages more or less unusable. |
shouldn't new features like this get a changelog entry? it seems like a lot of new features get introduced without them these days |
Not unusable, it just requires more manual work. I'm using ImportC to import ffmpeg headers, and manually translate the macros ImportC doesn't parse yet: import ffmpegh; // .c file with #include <libavcodec/avcodec.h> etc.
auto MKTAG(int a, int b, int c, int d) => (a | (b << 8) | (c << 16) | (cast(uint) d << 24));
enum AVERROR_EOF = -MKTAG( 'E','O','F',' '); ///< End of file I'm not objecting to this PR by the way. It's nice to reduce the amount of manual translations necessary. I'm only pointing out something to watch out for. |
@Herringway that's why I created the bugzilla issue for it, which should serve the purpose. |
It's probably technically possible to determine if the call to a converted macro will result in different semantics than a C preprocessor expansion, but it doesn't seem worth while, and may result in unwarranted confidence in the results :-/ |
@WalterBright If you want to go this route, how about letting the user write C code inside unit tests, and running the C preprocessor on that code before compiling it. The output could be compared with the output of D code that uses the same macro. That would at least provide a weak check for correctness, versus silently introducing bugs. |
Finally! Now macros like:
are rewritten as:
It's a miracle!